Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential switch to sparse matrices for monomial orderings #4114

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

HechtiDerLachs
Copy link
Collaborator

This is a draft for taking some first steps towards using sparse matrices for the canonical_matrix of a ModuleOrdering.

Today, @jankoboehm and myself investigated the caching of module gbs a little bit. We found that the production of the canonical_matrix for the ModuleOrderings is quite expensive, when the module has many generators (in our example 172). This is probably due to the matrix being dense. I tried to take some steps towards replacing it by a sparse matrix, but reached a point where I couldn't figure out any more how to proceed. However, I thought it might be useful to report on what I did so far and hence I make this draft PR. Let us eventually look into this together.

CC: @ederc @RafaelDavidMohr

@joschmitt joschmitt marked this pull request as draft September 19, 2024 10:12
@thofma
Copy link
Collaborator

thofma commented Sep 19, 2024

The old code has a lot of root for improvements (for example, not using is_zero but is_zero_entry). Do you have an example for benchmarking?

P.S.: I am asking, because 200x200 is not a size where I would expect large improvements from a sparse representation.

@HechtiDerLachs
Copy link
Collaborator Author

Do you have an example for benchmarking?

Sure:

R, (x, y, z) = QQ[:x, :y, :z]
F = FreeMod(R, 500)
I, _ = sub(F, gens(F))
@time standard_basis(I)
julia> @time standard_basis(I);
  2.762075 seconds (22.04 M allocations: 841.855 MiB, 36.02% gc time)

@joschmitt
Copy link
Member

I now have this timing:

julia> @time standard_basis(I);
  0.042802 seconds (320.76 k allocations: 15.418 MiB)

@joschmitt joschmitt force-pushed the switch_to_sparse_canonical_matrix branch from e6ee24e to 0002d78 Compare September 19, 2024 13:17
@joschmitt
Copy link
Member

If we want this, we would have to (also) allow sparse matrices in MatrixOrdering, so probably add a type parameter to the struct etc. I do not know whether it makes sense to store these matrices sparse; in my use cases they never get big.
The reason why it was so slow in the example wasn't necessarily the size of the matrix, but more the communication with the flint side see #4117.

@fingolfin
Copy link
Member

Let's merge PR #4117 first, as it is the more conservative (= safer? maybe?) change. Then we can consider whether and how to get this one here over the finishing line. Thank you for starting it in any case!!!

@fingolfin
Copy link
Member

@ederc will talk to @RafaelDavidMohr about this later (but Rafael is somewhat busy right now with defense prep)

@lgoettgens
Copy link
Member

This has some non-trivial conflicts. Someone (maybe @HechtiDerLachs) should try to resolve them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants